[Windows] Handle focus events using FocusManager (take 2)
#24695
Conversation
|
Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
mattleibow
added
t/a11y
MartyIX
force-pushed
the
feature/2024-09-10-Windows-Use-FocusManager-take4
branch
from
ee509b7 to
eab2340
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
It looks like Windows tests pass. |
| { | ||
| platformView.GotFocus += OnPlatformViewGotFocus; | ||
| platformView.LostFocus += OnPlatformViewLostFocus; | ||
| FocusManagerMapping.Add(platformView, this); |
There was a problem hiding this comment.
Is there no perf issues when there is a super complex UI? If we have maybe 1k elements on screen, will not the add and lookup be slow? It may still be faster than what we had before, but I am getting flashbacks when millions of objects would sub and unsub from the theme changed event.
There was a problem hiding this comment.
when millions of objects would sub and unsub from the theme changed event
Removing an entry from a CWT seems very fast compared to event unsubscription.
I would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).
If you're still concerned we have an alternative but much slower implementation:
#24355
There was a problem hiding this comment.
If we have maybe 1k elements on screen, will not the add and lookup be slow?
Adding to CWT is definitely faster because it does not involve interops (perf comparison of ConnectingHandler should be persuasive here). CTW should behave like a dictionary and "insert" operation for dictionaries is O(1) (i.e. so in practice it should be ~<10 nanoseconds1; whereas interops are more like milliseconds). So CWT lookups and inserts should be orders of magnitude faster.
I think that the CWT approach is strictly better for performance right now. What can be done is that I can try to create a huge grid with 1000 elements and measure performance for that scenario.
I would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).
That's a good idea.
Footnotes
-
https://startdebugging.net/2023/08/net-8-performance-dictionary-vs-frozendictionary/ just to get a feel what one might expect. ↩
There was a problem hiding this comment.
Perhaps @jonathanpeppers can double check my post as he's a performance guru :)
There was a problem hiding this comment.
I would rather set an initial capacity of 128 to the CWT to avoid a lot of initial resize operation (which is expensive).
That's a good idea.
@albyrock87 Interestingly, the CTW does not support passing capacity as far as I can tell: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2.-ctor?view=net-8.0 👀
MartyIX
force-pushed
the
feature/2024-09-10-Windows-Use-FocusManager-take4
branch
from
eab2340 to
4e65db7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| if (e.NewFocusedElement == view) | ||
| { | ||
| FocusManager.GotFocus -= OnFocused; | ||
| focusSource.SetResult(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think that could happen considering the event is being unsubscribed in the line above.
If there was a race condition, raising an exception would be exactly what we want in this device test.
There was a problem hiding this comment.
Ooh my bad, I didn't see it's a unit test. Reviewing code from mobile is hard 😅
There was a problem hiding this comment.
Btw: are you interested in this PR for performance reasons?
There was a problem hiding this comment.
@MartyIX yes, and it looks like to have a good gotcha on here, so I'm interested in learning new tricks (;
|
@rmarinho The mac tests still seem to fail. Is it OK? Or should this PR be rebased? |
MartyIX
deleted the
feature/2024-09-10-Windows-Use-FocusManager-take4
branch
* WIPWIP * Feedback * Refactor usage of `FocusManager` to avoid weak table usage * Use ConditionalWeakTable --------- Co-authored-by: Alberto Aldegheri <[email protected]>
samhouts
added
fixed-in-net9.0-nightly
samhouts
added
fixed-in-9.0.12
fixed-in-net8.0-nightly
Description of Change
This PR is the same as #24355 except that it uses
ConditionalWeakTableinstead of dependency properties.Subscribing and unsubscribing focus events for each MAUI view is costly.
This PR attempts to use
FocusManagerevents to achieve the same result.Performance impact
-> ∞ improvement :)
Issues Fixed
Contributes to #21787
cc @albyrock87